Skip to content

fix: stop y-websocket connection leaks in shared editing and reap idle /rtc connections at the gateway#6240

Open
aicam wants to merge 3 commits into
apache:mainfrom
aicam:fix/y-websocket-connection-leak
Open

fix: stop y-websocket connection leaks in shared editing and reap idle /rtc connections at the gateway#6240
aicam wants to merge 3 commits into
apache:mainfrom
aicam:fix/y-websocket-connection-leak

Conversation

@aicam

@aicam aicam commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Fixes two distinct y-websocket (/rtc) connection leaks in the shared-editing code path, plus a gateway-side idle-connection reaper as defense-in-depth. Both leaks live on SharedModelWebsocketProvider, and together they let the number of open realtime connections grow far out of proportion to the number of users until the shared-editing connection cap is exhausted and collaborative cursors/awareness stop syncing.

Leak B — throwaway validation graph opens a socket per edit (dominant).
On every operator/link/property edit, WorkflowCompilingService builds a throwaway WorkflowGraph via ValidationWorkflowService.getValidTexeraGraph() just to produce a logical plan for compilation. WorkflowGraph's constructor unconditionally creates a SharedModel, which unconditionally opens a new auto-connecting /rtc WebSocket in a random room. That graph is read once and never destroyed, so each edit leaks a socket that reconnects forever. A single user editing a workflow accumulates dozens of permanent sockets.

Fix: thread an enableSharedEditing flag from WorkflowGraph into SharedModel. When false, SharedModel skips the WebsocketProvider entirely and uses a standalone Awareness so downstream reads of awareness/clientId keep working. ValidationWorkflowService.getValidTexeraGraph() now passes false, so validation/compilation graphs never touch the network.

Leak A — incomplete provider teardown (per-navigation).
SharedModel.destroy() called wsProvider.disconnect() (and only when the socket happened to be OPEN). In y-websocket@1.4.0, disconnect() does not clear the provider's _checkInterval/_resyncInterval reconnect timers, so the provider is never garbage-collected and, if it was mid-reconnect at destroy time (common during workflow switches), keeps re-opening a zombie socket for the life of the tab.

Fix: call wsProvider.destroy() unconditionally, which clears those timers, removes the beforeunload handler, broadcasts awareness removal (peers drop our cursor), and closes the socket.

Supporting frontend changes. wsProvider is now optional (WebsocketProvider | undefined), so its remaining access sites in WorkflowActionService (setTempWorkflow / resetTempWorkflow) use ?., and the mock-workflow-plan.ts fixtures pass enableSharedEditing = false so unit tests open no sockets. Behavior for the real collaborative graph is unchanged: it still constructs a WebsocketProvider exactly as before.

Gateway change — reap idle /rtc connections (defense-in-depth).
The y-websocket reference server is single-process and every browser tab holds a long-lived WebSocket, so abandoned/leaked realtime sockets can pile up against the gateway's per-cluster max_connections circuit breaker (Envoy's default is 1024) and starve legitimate co-editing traffic. This PR gives the /rtc path its own HTTPRoute (so a policy can target just that cluster without affecting the other static routes) and attaches an Envoy Gateway BackendTrafficPolicy that:

  • drops upstream connections idle for longer than connectionIdleTimeout (default 5m), and
  • raises the per-cluster circuitBreaker.maxConnections (default 8192) for headroom.

Both knobs are configurable under yWebsocketServer.trafficPolicy in values.yaml; setting trafficPolicy to null omits the policy. This is complementary to the frontend fix — the frontend fix stops creating the leaked sockets; the gateway policy caps the blast radius of any that still linger.

Files changed:

  • frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts — optional/local-mode provider; correct destroy().
  • frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.ts — thread enableSharedEditing.
  • frontend/src/app/workspace/service/validation/validation-workflow.service.ts — pass false for the throwaway graph.
  • frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.tswsProvider?. null-safety.
  • frontend/src/app/workspace/service/execute-workflow/mock-workflow-plan.ts — fixtures open no sockets.
  • bin/k8s/templates/base/gateway/gateway-routes.yaml — split /rtc into its own HTTPRoute.
  • bin/k8s/templates/base/shared-editing-server/shared-editing-server-backend-traffic-policy.yaml — new BackendTrafficPolicy.
  • bin/k8s/values.yaml, bin/k8s/values-development.yamlyWebsocketServer.trafficPolicy knobs.

Any related issues, documentation, discussions?

Fixes #6239.

How was this PR tested?

Existing frontend unit tests continue to pass; the mock-workflow-plan.ts fixtures were updated to construct their graphs local-only so tests no longer open real /rtc sockets.

Manual verification, client-side (browser DevTools → Network → WS filter):

  1. Open a workflow → exactly one /rtc WebSocket (unchanged).
  2. Add/delete operators, edit properties, add/remove links repeatedly → the count stays at one (before this change, a new socket appeared on every edit and kept reconnecting).
  3. Switch workflow A → B → the old socket closes and one new socket opens (count stays 1).
  4. Navigate to the dashboard / close the tab → the socket closes (count → 0), with no lingering reconnects.

The same behavior is observable server-side by counting established connections to the y-websocket server while a single user edits: after this change the count stays flat (≈ number of open collaborative tabs) and drops when tabs close, instead of climbing with every edit.

Gateway change: helm template was run to confirm the chart renders valid manifests — the /rtc HTTPRoute and the BackendTrafficPolicy (with connectionIdleTimeout and circuitBreaker.maxConnections) are emitted with the policy correctly targeting only the shared-editing route, and the policy is omitted when yWebsocketServer.trafficPolicy is null.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

Two distinct leaks on the SharedModel -> WebsocketProvider path exhausted
the shared-editing (/rtc) connection cap and broke collaborative cursors:

- Per-edit (dominant): the throwaway WorkflowGraph built for validation on
  every edit unconditionally created a SharedModel, opening a new /rtc socket
  that was never destroyed and reconnected forever. Thread an enableSharedEditing
  flag through WorkflowGraph -> SharedModel and pass false from
  ValidationWorkflowService.getValidTexeraGraph() so validation/compilation
  graphs run local-only (standalone Awareness, no WebsocketProvider).

- Per-navigation: SharedModel.destroy() called wsProvider.disconnect() (only
  when connected), which leaves y-websocket's reconnect timer running so the
  provider is never GC'd. Call wsProvider.destroy() unconditionally instead.

wsProvider is now optional; add ?. at its remaining access sites and pass
enableSharedEditing=false in mock fixtures so unit tests open no sockets.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added fix frontend Changes related to the frontend GUI labels Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @aglinxinyuan, @bobbai00
    You can notify them by mentioning @aglinxinyuan, @bobbai00 in a comment.

Give the shared-editing (y-websocket) /rtc path its own HTTPRoute and attach
a BackendTrafficPolicy that drops idle upstream connections and raises the
per-cluster max_connections circuit breaker above Envoy's 1024 default.

The y-websocket reference server is single-process and every browser tab holds
a long-lived WebSocket, so abandoned/leaked realtime sockets can pile up against
the gateway's per-cluster connection cap and starve legitimate co-editing
traffic. This is defense-in-depth for the client-side connection leak fixed in
the frontend. Both the idle timeout (default 5m) and the connection ceiling
(default 8192) are configurable via yWebsocketServer.trafficPolicy; set it to
null to omit the policy.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the infra label Jul 7, 2026
@aicam aicam changed the title fix(frontend): stop y-websocket connection leaks in shared editing fix: stop y-websocket connection leaks in shared editing and reap idle /rtc connections at the gateway Jul 7, 2026
@codecov-commenter

codecov-commenter commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.75000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.62%. Comparing base (7c135ee) to head (9cedc6f).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...space/service/workflow-graph/model/shared-model.ts 70.00% 2 Missing and 1 partial ⚠️
...ce/workflow-graph/model/workflow-action.service.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6240      +/-   ##
============================================
+ Coverage     60.46%   60.62%   +0.15%     
  Complexity     3370     3370              
============================================
  Files          1133     1133              
  Lines         43985    44142     +157     
  Branches       4782     4827      +45     
============================================
+ Hits          26596    26760     +164     
+ Misses        15937    15929       -8     
- Partials       1452     1453       +1     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 1ec9e67
agent-service 44.59% <ø> (ø) Carriedforward from 1ec9e67
amber 66.04% <ø> (ø) Carriedforward from 1ec9e67
computing-unit-managing-service 0.00% <ø> (ø) Carriedforward from 1ec9e67
config-service 52.30% <ø> (ø) Carriedforward from 1ec9e67
file-service 63.97% <ø> (ø) Carriedforward from 1ec9e67
frontend 52.42% <68.75%> (+0.45%) ⬆️
notebook-migration-service 78.57% <ø> (ø) Carriedforward from 1ec9e67
pyamber 91.18% <ø> (ø) Carriedforward from 1ec9e67
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from 1ec9e67

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses runaway /rtc (y-websocket) WebSocket growth caused by frontend shared-editing objects being created without proper teardown (and, in one path, created unintentionally for throwaway validation graphs). It also adds an Envoy Gateway policy as defense-in-depth to reap idle /rtc upstream connections and raise the connection ceiling for that cluster.

Changes:

  • Added an enableSharedEditing switch so throwaway/validation WorkflowGraph instances do not create a WebsocketProvider (avoids per-edit leaked /rtc sockets).
  • Fixed shared-editing teardown by destroying the provider (wsProvider.destroy()) rather than only disconnecting, eliminating reconnect-timer “zombie” providers.
  • Split /rtc into its own Gateway HTTPRoute and added an optional BackendTrafficPolicy (idle timeout + increased maxConnections) configurable via Helm values.

Reviewed changes

Copilot reviewed 5 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts Makes wsProvider optional, supports local-only mode, and ensures provider is fully destroyed on teardown.
frontend/src/app/workspace/service/workflow-graph/model/workflow-graph.ts Threads enableSharedEditing into SharedModel construction to avoid unintended network connections.
frontend/src/app/workspace/service/validation/validation-workflow.service.ts Builds validation graphs with enableSharedEditing=false to prevent per-edit WebSocket leaks.
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts Updates provider access for optionality (but one call site still needs full null-safe access).
frontend/src/app/workspace/service/execute-workflow/mock-workflow-plan.ts Updates fixtures to create local-only graphs (no sockets during tests/fixtures).
bin/k8s/templates/base/gateway/gateway-routes.yaml Moves /rtc to its own HTTPRoute so policies can target it without affecting other routes.
bin/k8s/templates/base/shared-editing-server/shared-editing-server-backend-traffic-policy.yaml Adds optional Envoy Gateway BackendTrafficPolicy for /rtc (idle reaping + circuit breaker headroom).
bin/k8s/values.yaml Adds yWebsocketServer.trafficPolicy knobs (connectionIdleTimeout, maxConnections).
bin/k8s/values-development.yaml Mirrors the new yWebsocketServer.trafficPolicy knobs for dev values.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +781 to 783
if (this.texeraGraph.sharedModel.wsProvider?.shouldConnect) {
this.texeraGraph.sharedModel.wsProvider.disconnect();
}

@aglinxinyuan aglinxinyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You manually verified the behavior after the fix. Could you also verify it before the fix? Specifically, we'd like to confirm that there were consistently multiple WebSocket connections before this change.

Also, there's no need to list the files changed in the PR description.

Envoy Gateway's BackendTrafficPolicy exposes the connection idle timeout under
spec.timeout.http.connectionIdleTimeout; spec.timeout.tcp only has connectTimeout.
The previous timeout.tcp.connectionIdleTimeout was rejected by the CRD schema
("field not declared in schema"), which blocked the whole release from applying.
Verified against a live Envoy Gateway install on minikube.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Yicong-Huang

Copy link
Copy Markdown
Contributor

do we need to backport the fix to v1.2?

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a enableSharedEditing config? Please find a better way to pass the config. Bundling it with WorkflowGraph is not ideal.

validOperators,
validLinks,
texeraGraph.getAllCommentBoxes(),
/* enableSharedEditing */ false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 on this change. WorkflowGraph should not describe if shared editing is enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix frontend Changes related to the frontend GUI infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Frontend leaks y-websocket (/rtc) connections, exhausting the shared-editing connection cap and breaking collaborative cursors

5 participants